Add some tracing and logging#1621
Conversation
|
Could you also remove all default features besides "fmt" from tracing subscriber? It wont do anything now but will have add compile time improvement later. |
Already done in a later commit, which had not yet pushed. But yeah, it's a good point. |
PoignardAzur
force-pushed
the
tracing
branch
from
7353ab4 to
5b4ce67
Compare
cmyr
left a comment
cmyr
left a comment
There was a problem hiding this comment.
First: thank you or your patience so far, this has definitely been a trickier set of patches than we thought, at first.
Second: I don't think we're quite there, yet.
Having to instrument each widget independently feels awkward. Ultimately, widgets are generally not where the important stuff happens (the exception being the container widgets, and complex things like scrolling). Since all of the logic around invalidation and event delivery is scoped to the WidgetPod, it feels to me like we should be able to get most of the benefits of this patch just be instrumenting WidgetPod, and then also instrumenting the various Ctx methods.
There's a method on the Widget trait, type_name. This returns &str with a verbose type name, like: druid::widget::radio::Radio<druid::widget::flex::MainAxisAlignment>. I think that we could use this method, in combination with WidgetPod, to essentially instrument all widgets, in one place.
Did you consider this approach? Are there some limitations that I've overlooked?
Paraphrasing what I said on zulip for visibility: Okay, so I've tried the Basically, if I have This is because tracing only accepts string literals as span names; as I understand it, the goal is to cache is whether or not to use a given span at startup, to avoid doing string prefix comparison every time the program needs to compare a span against filter rules. So I'll keep the |
cmyr
left a comment
cmyr
left a comment
There was a problem hiding this comment.
Okay so:
Ultimately I think there are clear arguments for having most of the tracing being on WidgetPod, but if that is too difficult then I understand wanting to prioritize getting something working now, that is reasonably effective.
Thank you for your patience @PoignardAzur!
It doesn't seem like a code path that warrants a warning message.
5 participants
Copy-pasting from Zulip:
I'm still using
#[instrument]because, as I've said earlier, this is far from the biggest compile bottleneck right now; (I'm going to do some tests again to measure the impact it has). Also, while I don't like the syntax instrument has right now (having to add skip(...) everywhere), apparently it's due to be changed in the next version of tracing.Right now I'm trying to address the use-case that made me want to add more logging: being able to tell when an internal event (warning, something being redrawn, etc) happens and why.
Part of that is adding spans everywhere; since druid, like basically every GUI, uses a variation on the visitor pattern, that means most spans are going be named after the Self type of the function they instrument; except for the root span. For instance, if you have Window::event calling Flex::event calling Button::event, the context chain is going to look like event:Flex:Button.
I feel like I should document that last part somewhere.